New A72 changes for OpenTelemetry #8216 by vinothkumarr227 · Pull Request #8226 · grpc/grpc-go

vinothkumarr227 · 2025-04-04T06:28:43Z

RELEASE NOTES: None

codecov · 2025-04-04T06:33:27Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.06%. Comparing base (732f3f3) to head (ed45ee7).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8226      +/-   ##
==========================================
- Coverage   82.09%   82.06%   -0.04%     
==========================================
  Files         412      412              
  Lines       40491    40497       +6     
==========================================
- Hits        33242    33233       -9     
- Misses       5876     5891      +15     
  Partials     1373     1373

Files with missing lines	Coverage Δ
stats/opentelemetry/client_tracing.go	`84.00% <100.00%> (ø)`
stats/opentelemetry/server_tracing.go	`100.00% <100.00%> (ø)`
stats/opentelemetry/trace.go	`88.88% <100.00%> (+1.38%)`	⬆️

... and 26 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

purnesh42H

was there no change needed for Attempt? Is it already following grpc/proposal#474 ?

purnesh42H · 2025-04-08T04:03:47Z

stats/opentelemetry/trace.go

 * limitations under the License.
 */

+// OpenCensus's binary format for grpc-trace-bin:


no need to add this. This was only for grfc

vinothkumarr227 · 2025-04-08T07:16:32Z

was there no change needed for Attempt? Is it already following grpc/proposal#474 ?

The Attempt span name already follows the grpc/proposal#474 naming convention.
https://github.com/grpc/grpc-go/blob/master/stats/opentelemetry/client_tracing.go#L47

purnesh42H

lgtm

purnesh42H · 2025-04-08T07:48:35Z

@dfawley for second review

yashykt · 2025-04-08T19:23:22Z

stats/opentelemetry/trace.go

+		span.AddEvent("Outbound message", trace.WithAttributes(
 			attribute.Int64("sequence-number", int64(ai.countSentMsg)),
 			attribute.Int64("message-size", int64(rs.Length)),
 			attribute.Int64("message-size-compressed", int64(rs.CompressedLength)),


the grfc says to only add this if the message is actually compressed

yashykt · 2025-04-08T19:23:27Z

stats/opentelemetry/trace.go

+		span.AddEvent("Inbound message", trace.WithAttributes(
 			attribute.Int64("sequence-number", int64(ai.countRecvMsg)),
 			attribute.Int64("message-size", int64(rs.Length)),
 			attribute.Int64("message-size-compressed", int64(rs.CompressedLength)),


the grfc says to only add this if the message is actually compressed

gRFC says if compression is not a separate event, just add the key message-size-compressed in the same event "Inbound Message" and "Outbound Message". For go implementation, if compression is not enabled message-size-compressed is same as actual message. So, i guess what we have as of now is fine? or should we only add if compressed and actual are not equal?

The text says -
If compression needed, add key message-size-compressed with integer value of compressed message size.

Thanks. @vinothkumarr227 could you do similar to what Java has https://github.com/grpc/grpc-java/blob/f79ab2f16f7e037da0e8c9985e917eca552c22cb/opentelemetry/src/main/java/io/grpc/opentelemetry/OpenTelemetryTracingModule.java#L439 which is to add message-size-compressed only if compression was enabled.

Do for both inbound and outbound. For compression enabled, check if message-size is not equal to compressed length

grpc-go/stats/stats.go

Line 89 in 732f3f3

// CompressedLength is the size of the compressed payload data. Does not

@purnesh42H I have added both inbound and outbound. For compression to be considered enabled

yashykt · 2025-04-08T19:39:09Z

We also need tests for streaming, but that can be added in a subsequent PR

purnesh42H · 2025-04-09T05:07:57Z

message-size-compressed

the e2e tests contains tests for both unary and streaming RPCs. Are you referring to anything else?

purnesh42H · 2025-04-09T05:16:10Z

@vinothkumarr227 please resolve the conflicts from #8237

vinothkumarr227 · 2025-04-09T05:50:45Z

@vinothkumarr227 please resolve the conflicts from #8237

I have resolved the conflicts

yashykt · 2025-04-09T18:49:39Z

the e2e tests contains tests for both unary and streaming RPCs. Are you referring to anything else?

In all of the tests, I see only a single message being sent/received, which means that the logic on the sequence numbers incremented is untested.

purnesh42H · 2025-04-10T04:02:57Z

In all of the tests, I see only a single message being sent/received, which means that the logic on the sequence numbers incremented is untested.

okay. Filed the issue #8240

purnesh42H · 2025-04-10T09:12:24Z

stats/opentelemetry/trace.go

-			attribute.Int64("message-size-compressed", int64(rs.CompressedLength)),
-		))
+		}
+		if rs.CompressedLength > 0 && rs.CompressedLength != rs.Length {


nit: no need to check > 0?

purnesh42H · 2025-04-10T09:12:36Z

stats/opentelemetry/trace.go

-			attribute.Int64("message-size-compressed", int64(rs.CompressedLength)),
-		))
+		}
+		if rs.CompressedLength > 0 && rs.CompressedLength != rs.Length {


nit: no need to check > 0?

vinothkumarr227 added 2 commits April 4, 2025 05:25

Fixed the trace event cleanup

ad07d2d

Fixed the test cases

01927ac

Fixed the trace cleanup

8cd9b0b

purnesh42H reviewed Apr 8, 2025

View reviewed changes

purnesh42H assigned vinothkumarr227 Apr 8, 2025

purnesh42H added this to the 1.73 Release milestone Apr 8, 2025

purnesh42H added Type: Internal Cleanup Refactors, etc Area: Observability Includes Stats, Tracing, Channelz, Healthz, Binlog, Reflection, Admin, GCP Observability labels Apr 8, 2025

small tweaks

681ce72

vinothkumarr227 requested a review from purnesh42H April 8, 2025 07:20

purnesh42H approved these changes Apr 8, 2025

View reviewed changes

purnesh42H assigned dfawley and unassigned vinothkumarr227 Apr 8, 2025

yashykt approved these changes Apr 8, 2025

View reviewed changes

yashykt reviewed Apr 8, 2025

View reviewed changes

vinothkumarr227 added 2 commits April 9, 2025 10:48

Merge branch 'master' into a72-trace-cleanup

64124ff

small tweaks

6886a15

arjan-bal mentioned this pull request Apr 9, 2025

New A72 changes for OpenTelemetry #8216

Closed

trace: add message-size-compressed only when compression is enabled

ed45ee7

purnesh42H reviewed Apr 10, 2025

View reviewed changes

small tweaks

9a05b61

dfawley approved these changes Apr 16, 2025

View reviewed changes

dfawley assigned purnesh42H and unassigned dfawley Apr 16, 2025

purnesh42H merged commit 8b2dbbb into grpc:master Apr 17, 2025
13 checks passed

janardhanvissa pushed a commit to janardhanvissa/grpc-go that referenced this pull request Apr 23, 2025

New A72 changes for OpenTelemetry grpc#8216 (grpc#8226)

b391802

github-actions bot locked as resolved and limited conversation to collaborators Oct 15, 2025

Conversation

vinothkumarr227 commented Apr 4, 2025 • edited by purnesh42H Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Apr 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

purnesh42H left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vinothkumarr227 commented Apr 8, 2025

Uh oh!

purnesh42H left a comment

Choose a reason for hiding this comment

Uh oh!

purnesh42H commented Apr 8, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yashykt commented Apr 8, 2025

Uh oh!

purnesh42H commented Apr 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

purnesh42H commented Apr 9, 2025

Uh oh!

vinothkumarr227 commented Apr 9, 2025

Uh oh!

yashykt commented Apr 9, 2025

Uh oh!

purnesh42H commented Apr 10, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

vinothkumarr227 commented Apr 4, 2025 •

edited by purnesh42H

Loading

codecov bot commented Apr 4, 2025 •

edited

Loading

purnesh42H commented Apr 9, 2025 •

edited

Loading